@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Arthur Eubanks (aeubanks)

diff --git a/lld/ELF/OutputSections.cpp b/lld/ELF/OutputSections.cpp
index ee937418678707c..ab59432da7e8039 100644
--- a/lld/ELF/OutputSections.cpp
+++ b/lld/ELF/OutputSections.cpp
@@ -149,6 +149,16 @@ void OutputSection::commitSection(InputSection *isec) {
       error("incompatible section flags for " + name + "\n>>> " +
             toString(isec) + ": 0x" + utohexstr(isec->flags) +
             "\n>>> output section " + name + ": 0x" + utohexstr(flags));
+    if (config->emachine == EM_X86_64) {
+      if ((flags ^ isec->flags) & SHF_X86_64_LARGE) {
+        InputSection *conflictISec = getFirstInputSection(this);
+        warn("incompatible SHF_X86_64_LARGE section flag for " + name +
+             "\n>>> " + toString(conflictISec) + ": 0x" +
+             utohexstr(conflictISec->flags) + "\n>>> " + toString(isec) +
+             ": 0x" + utohexstr(isec->flags)
+        );
+      }
+    }
   }
 
   isec->parent = this;
diff --git a/lld/test/ELF/warn-mix-large-section.s b/lld/test/ELF/warn-mix-large-section.s
new file mode 100644
index 000000000000000..91fbeb5f64f6874
--- /dev/null
+++ b/lld/test/ELF/warn-mix-large-section.s
@@ -0,0 +1,25 @@
+# REQUIRES: x86
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/a.s -o %t/a.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/b.s -o %t/b.o
+# RUN: ld.lld %t/a.o %t/b.o -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK: warning: incompatible SHF_X86_64_LARGE section flag for foo
+# CHECK-NEXT: >>> {{.*}}a.o:(foo): 0x10000003
+# CHECK-NEXT: >>> {{.*}}b.o:(foo): 0x3
+
+#--- a.s
+.section foo,"awl",@progbits
+
+.type   hello, @object
+.globl  hello
+hello:
+.long   1
+
+#--- b.s
+.section foo,"aw",@progbits
+
+.type   hello2, @object
+.globl  hello2
+hello2:
+.long   1

✅ With the latest revision this PR passed the C/C++ code formatter.

I wonder if we should warn more broadly on mismatched section flags?

I understand your perspective, but the presence of a warning can be debated from both angles.

Issuing a warning allows us to capture certain compiler-side issues or user configuration problems within our control.

However, it is conventional to take the union of all input flags.
For example, merging a non-writable section with a writable section is supported, even in many cases undesired.
Therefore, the rationale behind this patch seems somewhat tenuous.

If a warning (an or error if --fatal-warnings) arises, and it's challenging to manage, should we incorporate
another option to suppress the warning/error?

If we add an option for this, then there is a question whether this is significant enough to justify a new option.
I'm undecided, considering the somewhat feeble basis.

We may actually want to consider an LLD warning suppression mechanism. LLD COFF and MSVC link.exe have a warning for mismatched section flags, and we should do the same for LLD. Typically, taking the union of R-- and RW- is OK, but merging RW- and R-X should be a warning/error, if it isn't already.

since warning on mismatched section flags in general is controversial, can we just proceed with this patch in its current form which only warns on mismatched SHF_X86_64_LARGE?

this would have helped me debug a medium code model/FDO instrumentation issue much quicker

ping

One concern I have with this change is that it will create linker warnings for instrumentation tools (PGO & ASan) with mixed code models.

For ASan, asan_globals is large under large/medium models. For PGO, __llvm_prof_names is large under large/medium models There are no direct references to these sections, so taking the union of flags in the linker is safe, and the warning is safe to ignore in this case.

Would it be overkill to anticipate this? Should we special case some sections out of this, or should we plan to create a general mechanism to suppress this warning for particular sections?

Yes. There are my concerns. Essentially, this is a lint feature disguised as a warning. However, if we reach a point where we need a hard-coded list, I think the diagnostic no longer pull its weight and should be removed instead.

I discovered that in https://reviews.llvm.org/D20651 for PGO instrumentation value profiling, there are static buffers allocated per-TU, the size of which is based on a heuristic of how much value profiling there will be, plus a fixed amount for the entire binary added by that patch. these aren't directly referenced by code and is a case where we actually do want the flag union behavior. this is another case of "non-directly-referenced globals that are only small in some TUs but is harmless to make large", like if we have some asan_globals sections in precompiled small code model code mixed with medium/large code model TUs

#78348 should resolve most of the cases where there's actually an issue

I'll probably not submit this patch, but it is useful for seeing if there are any instrumentation sections we've explicitly marked large when we shouldn't have

Essentially, this is a lint feature disguised as a warning.

I'm not sure if I agree with this phrasing. I would say this is a warning, but there are some false positive cases. The example Arthur used is to compile with ASan and mix large and small code model objects. Because there are no direct references to asan_globals, the warning is a false positive, it is safe to suppress, there will be no PC32 references to asan_globals.

I think this warning has enough value to land it. Would an option to specifically disable this warning (think -Wno-x, --no-warn-section-flag-mismatch, however we spell linker warning flags) address the concerns? Are we concerned about the performance cost of the conditional check, or the code complexity?

I think we can address the performance concerns by tweaking the conditionals. We have to check flag section mismatch, which is very rare, and we can outline everything after that as cold code with marginal performance impact.